test: improve fungible module coverage (system_contract tests) #1782
Conversation
skosito
requested review from
brewmaster012,
kingpinXD,
kevinssgh,
fadeev,
lumtis and
ws4charlie
as code owners
There was a problem hiding this comment.
This looks good. I will have a second look once we merge codecov
The general suggestion would be to change the name of the test function by using only the method name to be tested and give a description of the actual test, like "fail if not deploy" in a sub test description using t.Run
lumtis
changed the title
|
!!!WARNING!!! Be very careful about using Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203 Pay extra attention to the way |
|
@ws4charlie i replaced all occurences of deprecated sdk errors, and 0x0 comparisons i just pushed to this PR since it is safe change, please re-check when you get a chance |
Description
Adds tests for
system_contract.goin fungible module to improve coverage.I think we need separate issue to do some research/discussion on integration vs unit tests for keepers, when and how to write them, since they are a bit mixed up atm and sometimes we need to be able to mock to test how keeper code interacts with some of EvmCall failures. I tried using evm keeper mocks ApplyMessage in some tests, but I think it is very low level call to mock, and sometimes it is awkward since ApplyMessage is used in bunch of places. Also mocking things like abi unpack this way is very unclean (even though those are not so important).
Ideally it would be better to directly mock fungibleKeeper.EvmCall method, maybe we can open additional issue now when coverage is high, to try and write some of these failure tests in that way if possible and avoid mocking too low level calls?
Closes: #1742
Type of change
New tests
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Include instructions and any relevant details so others can reproduce.
Checklist: